-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow ext keys #3
Allow ext keys #3
Conversation
Bit of a wallet philosophy thing, but I think a watch key should not count as a private key. It's much more similar in functionality and security than an xpub. In other words, I think it should be possible to import these into a watch-only wallet. |
I did manage to succesfully import into a wallet with [
{
"success": true,
"warnings": [
"Not all private keys provided. Some wallet functionality may return unexpected errors"
]
}
] This can probably be avoided with the above. |
|
@Sjors I'm thinking about using the existing
Makes sense. I'll add a special case to |
I think that's a good idea; that had me confused as well. Perhaps when reusing a label, you could return the same address (maybe emit a warning). There's currently no convenient way to get all labels and their addresses - that might require a new RPC. |
Perhaps I did something wrong cherry-picking your commits, but trying to import into a watch-only wallet throws an error for me:
It seems to have added the descriptor twice, one marked as It started a rescan but it doesn't seem to make progress. Calling |
@Sjors Most of this stuff is still a work in progress so there's probably a lot of errors. The idea is to open PRs to josibake/implement-bip352-full with new functionality gradually. I'll investigate these errors that you have pointed out. Thank you for the early testing! |
Did you cherry-pick on top of josibake/implement-bip352-full? |
The previous commit added a test which would fail the unsigned-integer-overflow sanitizer. The warning is harmless and can be triggered on any commit, since the code was introduced. For reference, the warning would happen when the separator `-` was not present. For example: GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json would result in: rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long') #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77 #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9 #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12 #5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2) #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o #7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) bitcoin#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
@Eunovo I rebased the top commits from |
Strongly agree. We even talked about renaming scan key to "scan entropy," since it is not a private key (i.e. it is not used to secure funds). A scan key + spend pubkey (
I would expect Also, I saw this comment from @Sjors earlier but can't find it now, but if a sp descriptor is imported into a wallet, this should set the |
adcfa5d
to
87c1b71
Compare
@josibake and @Sjors I stopped storing the scan key to DB in 8ed833b since the desc always contains the scan key, we don't need store it separately. The scan key is also no longer added to the private key map after descriptor parsing, so the watch-only wallets should not have a problem with sp descs |
I'm still getting "Cannot import private keys to a wallet with private keys disabled" when importing an When importing an Loading a silent payment wallet still crashes with I like how it now produces the same address for the same label (with BIP329 we could also ensure that when restoring a wallet you get the same index->label mapping). I wonder how easy / hard it would be to get fast rescan working if the user has both |
- Stop adding scan key to singingprovider private key map, this will prevent scan key from being counted among private keys - Stop storing scan keys to db, scan keys are already saved in descriptor
DescriptorScriptPubKeyMan::MarkUnusedAddresses tries to expand the descriptor and read the resulting scripts This does not work for sp descriptors as no scripts are returned This leads to a SegFault
I wasn't able to reproduce this on faabf4e. See results below:
Note that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some comments/questions but going to go ahead and merge and start testing. Can address any comments in follow-ups
for (const auto& sppubkey_pair: provider.sppubkeys) { | ||
addresses.push_back(EncodeDestination(V0SilentPaymentDestination(sppubkey_pair.second))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 06281a3:
silent payments aren't ranged, so not clear to me how this line would be hit if calling deriveaddresses with an sp descriptor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code always runs at least once. See line 268
src/wallet/scriptpubkeyman.cpp
Outdated
V0SilentPaymentDestination main_dest; | ||
main_dest.m_scan_pubkey = sppubkey->scanKey.GetPubKey(); | ||
main_dest.m_spend_pubkey = sppubkey->spendKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in e81e28a:
Can simplify this to use the constructor, e.g.:
V0SilentPaymentDestination main_dest; | |
main_dest.m_scan_pubkey = sppubkey->scanKey.GetPubKey(); | |
main_dest.m_spend_pubkey = sppubkey->spendKey; | |
auto main_dest = V0SilentPaymentDestination{sppubkey->scanKey.GetPubKey(), sppubkey->spendKey}; |
// We only need to save the spend key | ||
// The scan key is preserved in the descriptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a065b4b:
What would be the harm in persisting the key to the database? I think this is fine, but I am suspicious there might be unintended consequences from not persisting the key. Will need to study this section a bit more, thought, to understand how descriptors are persisted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm that I can think of, It just requires more code. Currently, the wallet does not allow the persistence of private keys when the private keys are disabled. If we don't persist the scan key, this error isn't triggered on watch only wallets.
src/wallet/scriptpubkeyman.cpp
Outdated
CTxDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index) | ||
V0SilentPaymentDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 817e931:
why the change to V0SilentPaymentDestination? Seems like this was fine as a CTxDestination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reversed the change locally and it compiles successfully. I'm not sure why I changed it initially.
I'll let you know if I run into the crash in later updates. Haven't tested again yet. |
* rpc/deriveaddress: Add support for sp desc * spscriptpubkeyman: Move labelled destination generation to seperate methods * rpc/getnewaddress: Add silent-payment to addess_type list * descriptor: Allow extended keys in sp() * Fix scan key storage issues in watch only wallet - Stop adding scan key to singingprovider private key map, this will prevent scan key from being counted among private keys - Stop storing scan keys to db, scan keys are already saved in descriptor * spscriptpubkeyman: Fix MarkUnusedAddresses SegFault DescriptorScriptPubKeyMan::MarkUnusedAddresses tries to expand the descriptor and read the resulting scripts This does not work for sp descriptors as no scripts are returned This leads to a SegFault * test: Add E2E test for sp descriptor
rpc: deriveaddress
returns